fix: DH-22326: Un-hide group-by columns, keep other hidden columns hidden when applying rollup#2668
Open
vbabich wants to merge 3 commits intodeephaven:mainfrom
Open
fix: DH-22326: Un-hide group-by columns, keep other hidden columns hidden when applying rollup#2668vbabich wants to merge 3 commits intodeephaven:mainfrom
vbabich wants to merge 3 commits intodeephaven:mainfrom
Conversation
vbabich
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates iris-grid rollup handling so applying a rollup no longer unhides every hidden column. Instead, it targets just the selected group-by columns and relies on the grid metric calculator’s name-based width preservation during model swaps.
Changes:
- Replaced the
showAllColumns()reset inhandleRollupChangewith per-group-columnresetColumnWidthcalls. - Kept the existing rollup state reset/loading flow intact while narrowing the visibility change.
- Added unit tests for the new
handleRollupChangebehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/iris-grid/src/IrisGrid.tsx |
Changes rollup application to selectively unhide grouped columns instead of resetting all hidden columns. |
packages/iris-grid/src/IrisGrid.test.tsx |
Adds unit coverage for the new rollup column-width reset path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2668 +/- ##
==========================================
+ Coverage 49.78% 49.95% +0.16%
==========================================
Files 774 774
Lines 43917 43926 +9
Branches 11129 11132 +3
==========================================
+ Hits 21866 21945 +79
+ Misses 22033 21963 -70
Partials 18 18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+313
to
+319
| * @param name The name of the column to reset | ||
| */ | ||
| resetColumnWidthByName(name: ColumnName): void { | ||
| this.userColumnWidthsByName.delete(name); | ||
| if (this.cachedModelColumnNames != null) { | ||
| const index = this.cachedModelColumnNames.indexOf(name); | ||
| if (index >= 0) { |
Comment on lines
+269
to
+292
| it('un-hides group-by columns by name', () => { | ||
| const columns = irisGridTestUtils.makeColumns(3); | ||
| const irisGrid = makeComponent( | ||
| irisGridTestUtils.makeModel(irisGridTestUtils.makeTable({ columns })) | ||
| ); | ||
| const { metricCalculator } = irisGrid.state; | ||
| const resetColumnWidthByName = jest.spyOn( | ||
| metricCalculator, | ||
| 'resetColumnWidthByName' | ||
| ); | ||
|
|
||
| const groupByNames = [columns[1].name, columns[2].name]; | ||
|
|
||
| act(() => { | ||
| irisGrid.handleRollupChange({ | ||
| columns: groupByNames, | ||
| showConstituents: true, | ||
| showNonAggregatedColumns: true, | ||
| }); | ||
| }); | ||
|
|
||
| expect(resetColumnWidthByName).toHaveBeenCalledWith(groupByNames[0]); | ||
| expect(resetColumnWidthByName).toHaveBeenCalledWith(groupByNames[1]); | ||
| expect(irisGrid.state.rollupConfig?.columns).toEqual(groupByNames); |
Comment on lines
+332
to
+348
| // Simulate the column having been hidden previously (width 0 stored by name). | ||
| const newGroupByName = 'NotInCurrentModel'; | ||
| metricCalculator.userColumnWidthsByName.set(newGroupByName, 0); | ||
|
|
||
| // Spy AFTER seeding so the spy still calls through. | ||
| jest.spyOn(model, 'getColumnIndexByName').mockReturnValue(undefined); | ||
|
|
||
| act(() => { | ||
| irisGrid.handleRollupChange({ | ||
| columns: [newGroupByName], | ||
| showConstituents: false, | ||
| showNonAggregatedColumns: false, | ||
| }); | ||
| }); | ||
|
|
||
| expect(metricCalculator.userColumnWidthsByName.has(newGroupByName)).toBe( | ||
| false |
Collaborator
Author
|
Found a bug, fixing:
Expected:
Actual:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix DH-22326: when applying a rollup, group-by columns now become visible while previously hidden non-grouped columns stay hidden across the model swap. Replaces the previous
showAllColumns()reset inhandleRollupChangewith a targetedmetricCalculator.resetColumnWidthByNamecall for each group-by column, so the metric calculator's existing by-name width tracking handles preservation automatically. Also eliminates the brief flicker that the previous reset caused.